Repository navigation
Conversation
We need a way for querying the expired objects. This is it!
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #690 +/- ##
==========================================
+ Coverage 91.33% 91.48% +0.14%
==========================================
Files 117 118 +1
Lines 24171 24614 +443
==========================================
+ Hits 22077 22517 +440
- Misses 2094 2097 +3
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2e5604a. Configure here.
| let mut ids = Vec::new(); | ||
| for row in rows { | ||
| let id = row.clone(); | ||
| ids.push(ObjectId::from_storage_path(&id).unwrap()); |
There was a problem hiding this comment.
Expired listing panics on bad ids
Medium Severity
list_expired calls ObjectId::from_storage_path and unwraps. One stored path that does not round-trip — an empty key, empty usecase, or a row written outside this code — panics the whole listing instead of returning the Result the method already uses.
Reviewed by Cursor Bugbot for commit 2e5604a. Configure here.
aldy505
changed the title
|
|
||
| /// A change stream that can list expired objects. | ||
| #[async_trait::async_trait] | ||
| pub trait ExpiryIndex: ChangeStream { |
There was a problem hiding this comment.
I don't think we should mix the ExpiryIndex with the ChangeStream, rather, one should use the other.
I'm thinking we could model this as something like:
trait GcStorage: Send + Sync + Debug {
async fn upsert(&self, id: &ObjectId, expires_at: Option<Timestamp>) -> Result<(), ChangeStreamError>;
async fn remove(&self, id: &ObjectId);
async fn list_expired(&self, now: Timestamp, limit: u64) -> Result<Vec<ObjectId>, ChangeStreamError>;
}Then the GcChangeStream would depend on it, like so:
#[derive(Debug)]
struct GcChangeStream {
strorage: Arc<dyn GcStorage>
}
#[async_trait]
impl ChangeStream for GcChangeStream {
async fn begin_write(&self, id: &ObjectId, exp: Option<Timestamp>) -> Result<(), ChangeStreamError> {
self.storage.upsert(id, exp).await
}
// ...
}And similarly, you would construct construct your GC task that loops through the records and deletes stuff by passing it the same Arc<dyn GcStorage>.
What do you think?
There was a problem hiding this comment.
This makes more sense. But that means I need to change how I implemented it on #653
There was a problem hiding this comment.
Yeah, sorry about that but I only noticed this now that we're adding more capabilities to this struct.
Could you refactor it? 🙏
| &self, | ||
| now: Timestamp, | ||
| limit: u64, | ||
| ) -> Result<Vec<ObjectId>, Box<dyn std::error::Error + Send + Sync>> { |
There was a problem hiding this comment.
Normally we use thiserror which helps creating structured errors, I would prefer if we could use it here too, it could be something like ExpiryIndexError with the different variants.
| let mut ids = Vec::new(); | ||
| for row in rows { | ||
| let id = row.clone(); | ||
| ids.push(ObjectId::from_storage_path(&id).unwrap()); |
There was a problem hiding this comment.
Even though this should never happen, here we should log an error and continue, probably also deleting the bad row.
We certainly don't want to panic, and we also don't want to return an Error as Cursor suggests as that would stop the whole list_expired and you would find the same bad record on the next run.
…942) Closes #941. The changelog preview workflow didn't work in repos that define release workspaces. `craft changelog --pr N` failed because no workspace was selected, and the workflow hid the error, so every PR showed "No changelog entries" with no bump. Even with a workspace selected, the current PR was always added to the preview without the workspace path rules, so a server-only PR showed up as a bump in a client's changelog. **What changes** - If the config defines workspaces and none is selected, `craft changelog --pr N` previews the PR in each workspace, starting from that workspace's latest tag. It keeps only the workspaces whose changelog would list the PR. The JSON output gets a `workspaces` array (`name`, `bumpType`, `versioningPolicy`). `changelog` holds the PR's own entry once, since the entry text is the same in every workspace. - With a workspace selected, the PR's changed files (from the GitHub API, including the old path of renamed files) go through the same rules as the release changelog: owned paths, `includePaths`, `unownedPaths`. A PR outside the workspace is left out and doesn't bump the version. To support this, the path matching in `getChangesSince` moved into `getWorkspaceChangeFilter()` so both callers share it. - The workflow still posts one comment or one commit status. It lists each workspace the PR lands in with its bump (CalVer workspaces show `CalVer`), and the status description reads e.g. `clients/python: Minor, clients/rust: Patch`. When a PR lands in no workspace, the preview says so, and title enforcement is skipped as it is for `skip-changelog`. Repos without workspaces get the same output as before. I ran the built binary against the three-workspace config in getsentry/objectstore#737: - getsentry/objectstore#690 only changes `objectstore-service`. It now lands only in `objectstore-server` (CalVer). - getsentry/objectstore#721 lands only in `clients/python`, as a major bump. - getsentry/objectstore#728 is CI-only. Every workspace uses `unownedPaths: exclude`, so it lands in none. Callers that pin `craft-version` need a release with this change. The default `latest` picks it up. <!-- junior-request-attribution:start --> via @BYK. <!-- junior-request-attribution:end --> <!-- junior-session-footer:start --> <!-- junior-conversation-id:slack%3AC0CACQ70E8Z%3A1793125577.652579 --> -- [View Junior Session](https://junior-prod.sentry.dev/conversations/slack%3AC0CACQ70E8Z%3A1793125577.652579) [[Sentry]](https://sentry.sentry.io/explore/conversations/slack%3AC0CACQ70E8Z%3A1793125577.652579/?project=4510944073809921) <!-- junior-session-footer:end --> Co-authored-by: sentry-junior[bot] <264270552+sentry-junior[bot]@users.noreply.github.com> Co-authored-by: Burak Yigit Kaya <[email protected]>
We need a way for querying the expired objects. This is it!